Skip to content

Conversation

johankasperi
Copy link

@johankasperi johankasperi commented Jun 27, 2025

Description

The current implementation of headerLeft and headerRight adds a react view as a custom view in a UIBarButtonItem. This implementation is sufficient at most times but I believe we can achieve greater "native feel" if the native stack has an protocol for adding actual UIBarButtonItems in the header. As the UIBarButtonItems has properties and features that can be difficult to mimic with a react view.

Also with the introduction of iOS 26, using only custom views in UIBarButtonItem presents some limitations. Mainly that the adaptive tint color (based on the underlying view) is not working on a UIBarButtonItem with a custom view as demonstrated below under "Screenshots".

Changes

This PR adds the properties headerRightItems and headerLeftItems on the native stack Screen that makes it possible to add one or several UIBarButtonItem to the right/left of the header and/or functions returning a React Node. Most of the features of the UIBarButtonItem is supported (see either "Bar Button Items" in the example apps or the type definition).

Screenshots / GIFs

Non adaptive tint color when using old property headerRight on iOS 26

Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-06-27.at.16.31.10.mov

Adaptive tint color when using new property headerRightBarButtonItems on iOS 26

Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-06-27.at.16.28.30.mov

UIBarButtonItem with style "prominent" on iOS 26

Simulator Screenshot - iPhone 16 Pro - 2025-06-27 at 16 28 39

UIBarButtonItem with UIMenu on iOS 26

Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-06-27.at.16.29.02.mov

Test code and steps to reproduce

I've created a screen named "Bar Button Items" in the example app that showcases all of the proposed features.

Checklist

@johankasperi
Copy link
Author

johankasperi commented Jun 27, 2025

Related PR in react-navigation: react-navigation/react-navigation#12657

@Pnlvfx
Copy link

Pnlvfx commented Jul 2, 2025

i like this

@kkafar
Copy link
Member

kkafar commented Jul 18, 2025

Hey! This is really promising. We're working currently on support for iOS 26 etc. and we're doing general overhaul of the lib. As of now, we're not sure what will be the API shape & scope, therefore I won't review it for now. Once we settle the internal discussion I'll revisit your implementation. I'm sure we'll be able to land it in some form.

@johankasperi
Copy link
Author

johankasperi commented Aug 4, 2025

Hey! This is really promising. We're working currently on support for iOS 26 etc. and we're doing general overhaul of the lib. As of now, we're not sure what will be the API shape & scope, therefore I won't review it for now. Once we settle the internal discussion I'll revisit your implementation. I'm sure we'll be able to land it in some form.

I'm really glad you liked it! Yeah I saw that you have done a bunch of iOS 26 related PRs and changes to the API so I understand that my suggestion may not match the shape you are targeting. Please let me know if there are any changes I can do right now and I will happily do them asap. I truly believe something like this would be awesome for react-native-screens when iOS 26 is out of beta.

Copy link
Collaborator

@kmichalikk kmichalikk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi
Great stuff. We're looking at your PR and would like to merge the changes soon. Here are some comments from me, and I believe @kkafar wants to look more closely in the following days.

Also, please update the PR with changes from main branch. There could be some conflicts.

@johankasperi johankasperi force-pushed the ios-bar-button-item branch 2 times, most recently from e9b25c0 to f53da7d Compare August 6, 2025 10:00
@johankasperi
Copy link
Author

johankasperi commented Aug 6, 2025

Hi Great stuff. We're looking at your PR and would like to merge the changes soon. Here are some comments from me, and I believe @kkafar wants to look more closely in the following days.

Also, please update the PR with changes from main branch. There could be some conflicts.

Thank you! I've updated the PR with changes from main and resolved all conflicts

@kmichalikk kmichalikk requested a review from kkafar August 7, 2025 04:56
kmichalikk added a commit that referenced this pull request Aug 12, 2025
## Description

From discussion in
#2987 (comment).
We should rename the class into something more specific because it
really does one specific thing.

## Changes

title

## Test code and steps to reproduce

No changes, disabling back button menu should still work. You can modify
BottomTabsTest / Tab4 with `screenOption={{ headerBackButtonMenuEnabled:
false }}`
@kkafar
Copy link
Member

kkafar commented Aug 12, 2025

Hey, I most likely won't find time tomorrow & I'm going for a two week PTO on Thursday. I'll try to review this as one of first things after I'm back from PTO.

:sorry:

@johankasperi
Copy link
Author

Hey, I most likely won't find time tomorrow & I'm going for a two week PTO on Thursday. I'll try to review this as one of first things after I'm back from PTO.

:sorry:

Hey! No worries! Have a nice PTO 😊

@johankasperi
Copy link
Author

@kmichalikk I added support for systemImage (using SF Symbols) in this commit 8981d6a Feel free to take a look or try it out whenever you have the chance. Thanks!

@johankasperi
Copy link
Author

johankasperi commented Aug 19, 2025

@kmichalikk refactored everything a bit in af27795

I realised that users might want to combine native UIBarButtonItems and React Nodes in the header left and right. So renamed headerRightBarButtonItems and headerLeftRightBarButtonItems to headerRightItems and headerLeftItems. These two properties now accepts an array containing either dicts for UIBarButtonItems or functions returning a React Node. The new functionality is shown in the screen "ReactNodeButtonDemo".

Copy link
Collaborator

@kmichalikk kmichalikk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't able to properly run this probably because of some issues with linking to react-navigation PR - buttons were not displayed at all. I'll try again later. In the meantime I have two small nitpicks.

@johankasperi
Copy link
Author

I wasn't able to properly run this probably because of some issues with linking to react-navigation PR - buttons were not displayed at all. I'll try again later. In the meantime I have two small nitpicks.

My bad! I forgot to push my latest commits to react-navigation. Should be working now

@johankasperi
Copy link
Author

@kmichalikk I added support for hidesSharedBackground: boolean when rendering custom React Elements. Making it possible to show them without the liquid glass effect (see screenshot below). Feel free to have a look and try it out!

Simulator Screenshot - iPhone 16 Pro - 2025-09-02 at 12 05 26

Copy link
Collaborator

@kmichalikk kmichalikk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍
Some nitpick & suggestions below.
I pulled the PR locally and merged main, works fine but found some visual bugs that I suspect are native, but haven't checked that yet.

Comment on lines 147 to 152
#if defined(__IPHONE_OS_VERSION_MAX_ALLOWED) && defined(__IPHONE_26_0) && \
__IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_26_0
if (@available(iOS 26.0, *)) {
self.style = UIBarButtonItemStyleProminent;
}
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For iOS < 26 there is no style set explicitly here; maybe, to be consistent, we should set it to something? And please mention the difference for iOS versions somewhere in the docs.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to set it to something else on iOS < 26. I believe it's better to leave it untouched and use the default style of the OS when the prominent style is not available. UIBarButtonItemStylePlain is the default style of iOS < 26.

Copy link
Author

@johankasperi johankasperi Sep 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And about docs, I'm linking to the Apple docs in the JSDoc comments in the type declaration. I'm also mentioning that "Prominent" is only available on iOS 26. https://github.com/johankasperi/react-navigation/blob/668b77545440c51f473bf7e392a6b8b946eec8f9/packages/native-stack/src/types.tsx#L729

Could you point me to where in the docs I should also mention this?

One could also argue that linking to the iOS docs is sufficient, especially during the beta period when stuff might be changed by Apple, since by trying to copy Apples docs to the React Native Screen docs the RNS docs might become dated quickly when Apple are doing changes. But I'm not gonna be the judge of that 😊

@@ -0,0 +1,412 @@
// NOTE: The full native feature set (style, image, menu, etc.) is available, but the TS types in src/types.tsx need to be updated to match. This example uses only the currently typed props (title, icon, onPress, enabled).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case we should add some documentation to src/types.tsx, maybe also to GUIDE_FOR_LIBRARY_AUTHORS, especially the different cases when the behavior is different for iOS 26 vs 18. I saw there is more documentation in the PR for react-navigation, but it would be nice to have it also here in screens.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean this interface?

export interface ScreenStackHeaderConfigProps extends ViewProps {

And in GUIDE_FOR_LIBRARY_AUTHORS, do you mean I should add it under this section? https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md#screenstackheaderconfig

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I was thinking about, It'd be good to have something in the same place where other screens components' props are defined for quick reference when someone new jumps in. IN GUIDE... there is this section "Below is a list of properties that can be set with ScreenStackHeaderConfig component:" and since you are adding new props, we should keep it in sync & write something there. Same for src/types.tsx.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thansk for pointing me to where I should write something. I will do that asap

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the types in this commit 3e2f63d

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added docs in this commit b41acf8

@johankasperi johankasperi force-pushed the ios-bar-button-item branch 2 times, most recently from 03ed736 to 49fb1b8 Compare September 4, 2025 21:03
@johankasperi
Copy link
Author

Hey, I'm not really able to build FabricExample anymore locally:

image image
and it seems that CI also fails to do it. Could you restore it, so that I can test the changes?

Thats weird. I'm able to build it locally. Found an incorrect import in JS (69f6383) but that shouldn't prevent it from building right? Which Xcode are you using?

@johankasperi
Copy link
Author

johankasperi commented Oct 2, 2025

Hey, I'm not really able to build FabricExample anymore locally:

image image
and it seems that CI also fails to do it. Could you restore it, so that I can test the changes?

Realize now that no cpp files for RNSBarButtonItemCustomView are created. So RNSBarButtonItemCustomViewComponentDescriptor.h, RNSBarButtonItemCustomViewShadowNode.h and RNSBarButtonItemCustomViewShadowNode.cpp. Maybe thats why you and CI is unable to compile? How do I create them? Manually or with codegen?

@johankasperi
Copy link
Author

Hey, I'm not really able to build FabricExample anymore locally:
image image
and it seems that CI also fails to do it. Could you restore it, so that I can test the changes?

Realize now that no cpp files for RNSBarButtonItemCustomView are created. So RNSBarButtonItemCustomViewComponentDescriptor.h, RNSBarButtonItemCustomViewShadowNode.h and RNSBarButtonItemCustomViewShadowNode.cpp. Maybe thats why you and CI is unable to compile? How do I create them? Manually or with codegen?

But the cpp files is only needed if we want custom shadow nodes for RNSBarButtonItemCustomView, right? So they shouldn't be needed? If so then I don't know what is causing your problem and I would really appreciate some help. Did you run pod install?

@kkafar
Copy link
Member

kkafar commented Oct 6, 2025

Dunno what changed, but it runs fine now. I did remember to reinstall pods on Friday. The CI also failed back then. We'll see whether it passes now or not.

Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review part one. I'm submitting partially done one, cause I had to redo it twice as apparently it is not completely safe yet to refresh the site with "new review experience" on GitHub 😅

#### `ScreenStackHeaderRightView` / `ScreenStackHeaderLeftView`
The children will render on the right-hand or left-hand side of the navigation bar (or on the opposite side in case LTR locales are set on the user's device).

To configure `ScreenStackHeaderRightView` or `ScreenStackHeaderLeftView` use `<BarButtonItemCustomView<` as a child. `<BarButtonItemCustomView>` component that comes from react-native-screens supports these properties:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what it means exactly yet, but we need to make sure that this PR does not modify currently existing API in breaking manner.

(TODO: revisit this comment after full review)

Copy link
Author

@johankasperi johankasperi Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't modify existing API. It adds to the existing API by giving the opportunity to render a < BarButtonItemCustomView> (that has the prop hidesSharedBackground ) as a direct child to ScreenStackHeaderRightView or ScreenStackHeaderLeftView

If you look at my PR to react-navigation/native-stack I've added a BarButtonitemCustomView as direct child when rendering headerLeftItems/headerRightItems React Elements but not when rendering the existing headerLeftElement/headerRightElement. So it is optional https://github.com/johankasperi/react-navigation/blob/a79b931cfd1c4507db4ff794a0fe8f30579b439b/packages/native-stack/src/views/useHeaderConfigProps.tsx#L331


To configure `ScreenStackHeaderRightView` or `ScreenStackHeaderLeftView` use `<BarButtonItemCustomView<` as a child. `<BarButtonItemCustomView>` component that comes from react-native-screens supports these properties:

- `hidesSharedBackground?: boolean` - Hide shared background (iOS 26+). Read more: https://developer.apple.com/documentation/uikit/uibarbuttonitem/hidessharedbackground
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a reason for this prop to not live directly on the header subview components. Why complicate it here?

What do you think?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my initial thought but decided not to do that because the native RNSScreenStackHeaderSubview is used by more subviews than header left/right (center/title and search bar). And the hidesSharedBackground prop would have been obsolete and no effect on them. And as I said earlier I took inspiration from how RNSSearchBar was created and used, as a direct child to the RNSScreenStackHeaderSubview. So I think my implementation follows the existing API design/structure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point referring to the search bar here. I dislike distinguishing "custom subview" type from the rest of the API, however. Until now a way to render a custom view inside a header was to wrap it with ScreenStackHeaderSubview{Left,Rigth,Center} component. Most likely search bar was treated special, because it was a fully native view, w/o any react views inside, managed by the native side. Right now, we would create new way to render a custom view, IMO unnecessarily creating new ways of doing it. While I get that the hidesSharedBackground prop will not apply to all the possible contents I still think that its better way to go. Please note, that e.g. you could render SearchBar inside BarButtonInCustomView and still misuse the API, possibly breaking searchbar.
I want the hidesSharedBackgroundProp to live directly on header subview. In context where its application does not make sense - it should be a noop.

I feel like we could bikeshed here for a long time, therefore let's settle with putting in on ScreenStackHeaderSubview.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thanks for taking a clear decision on the matter and guiding me forward. It's not like I have a strong opinion on the matter and I totally respect that you have way more understanding of the entire lib than I have. In my previous response I just wanted explain why I designed it like I did and my thought process. I will add hidesSharedBackground directly to ScreenStackHeaderSubview asap so we can put an end to this discussion 😄

Copy link
Author

@johankasperi johankasperi Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have now moved the hidesSharedBackground to ScreenStackHeaderSubview in this commit b8a0164

I removed the CustomViewBarButtonItem in this commit 96c9846

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks!


To configure `ScreenStackHeaderRightView` or `ScreenStackHeaderLeftView` use `<BarButtonItemCustomView<` as a child. `<BarButtonItemCustomView>` component that comes from react-native-screens supports these properties:

- `hidesSharedBackground?: boolean` - Hide shared background (iOS 26+). Read more: https://developer.apple.com/documentation/uikit/uibarbuttonitem/hidessharedbackground
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also:

I understand that this prop allows to hide the background from header items created via already existing API, not the one this PR adds.

I like this change & I'm for it. But this should really be done in separate PR.

Keep this at low priority though. I'll let is slip if we're ready with rest of the PR.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#2987 (comment)

I doesn't break the existing API but the BarButtonItemCustomView component was needed to hide the glass effect background on bar button items with a custom view/React Element.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally get that this should have been done in a separate PR. Sorry about that


To configure `ScreenStackHeaderRightView` or `ScreenStackHeaderLeftView` use `<BarButtonItemCustomView<` as a child. `<BarButtonItemCustomView>` component that comes from react-native-screens supports these properties:

- `hidesSharedBackground?: boolean` - Hide shared background (iOS 26+). Read more: https://developer.apple.com/documentation/uikit/uibarbuttonitem/hidessharedbackground
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand this section correctly (have not read the rest of the recent changes yet), it introduces API to modify header items achievable with currently existing API.

While I like this idea, it should not be in this PR. Treat is as low prio though. If we sort out the rest of the PR I'll let it slip 😄

Comment on lines 52 to 56
if (_barButtonItem != nil) {
dispatch_async(dispatch_get_main_queue(), ^{
[self->_barButtonItem setHidesSharedBackground:self->_hidesSharedBackground];
});
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I ask for a bit of explanation why is the setter deferred here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't setHidesSharedBackground run on the JS thread (using old arch) but changing the visuals of the UIBarButtonitem need to be done on the main thread

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried to verify this when running examples app with old arch. setHidesSharedBackground is running on the main thread so no need to dispatch to it. Pretty sure I saw a crash here before but must have misunderstood it. Remove it here 2ecc9cf

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for verifying it. The mounting stage code (including prop updates) on both archs should be confined to UI thread, therefore we should be safe here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can navigate to this commit & see the changes, but when reviewing I still see the old code. Is this GitHub glitch or was this commit lose somewhere in rebase?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird. I will look into that asap

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must be a Github glitch because I'm not seeing the old code?

Comment on lines 641 to 645
UIBarButtonItem *buttonItem = [[UIBarButtonItem alloc] initWithCustomView:subview];
navitem.leftBarButtonItem = buttonItem;
if (subview.subviews.count > 0 && [subview.subviews[0] isKindOfClass:[RNSBarButtonItemCustomView class]]) {
RNSBarButtonItemCustomView *customView = subview.subviews[0];
[customView setUIBarButtonItem:buttonItem];
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dislike the approach here. We already create & initialise the button item in line 631. With introduction of RNSBarButtonItemCustomView we split part of the configuration to the child, which retains the bar button strongly, creating circular dependency, etc.

What about moving this prop directly onto header subview component & adding either:

  1. small factory method that will take the subview & output nicely configured UIBarButtonItem for us,
  2. or a subclass of UIBarButtonItem that will have initialiser ~ initWithHeaderSubview:(RNSScreenStackHeaderSubiew *) and will do nice configuration there.

Copy link
Author

@johankasperi johankasperi Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is with your suggestions the UIBarButtonItem will not be updated if the RNSScreenStackHeaderSubiew (or RNSBarButtonItemCustomView for that matter) props changes and rerenders. From what I understand we need to have a updateProps (for Fabric) and a reference to the existing UIBarButtonItem to change its props since it will not be recreated (as it was earlier when the configuration was part of the headerRightItems/headerLeftItems array).

What I'm considering now is ditching the idea of RNSBarButtonItemCustomView and go back to configuring the custom elements in the headerRightItems/headerLeftItems array. What do you think?

Copy link
Author

@johankasperi johankasperi Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I refactored RNSBarButtonItemCustomView in 3ef0f53 and removed the circular dep.

I also created a small factory method in d93acd6

Please let me know if you like this idea with RNSBarButtonItemCustomView or if we should ditch it in favour of using config in the items array instead. I still think what I said in #2987 (comment) is valid. Using RNSBarButtonItemCustomView is more similar to how RNSSearchBar is implemented. Thank you!

Comment on lines 655 to 658
if (subview.subviews.count > 0 && [subview.subviews[0] isKindOfClass:[RNSBarButtonItemCustomView class]]) {
RNSBarButtonItemCustomView *customView = subview.subviews[0];
[customView setUIBarButtonItem:buttonItem];
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, the same remarks apply here.

Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments regarding screenId on HeaderConfig

Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also like to discuss the approach to "buttons". Is there an advantage of creating "custom press detection" (mainly events infrastructure). Do you know whether simply letting uses use Pressable / Touchable elements inside would come with any disadvantages?

@johankasperi
Copy link
Author

I'd also like to discuss the approach to "buttons". Is there an advantage of creating "custom press detection" (mainly events infrastructure). Do you know whether simply letting uses use Pressable / Touchable elements inside would come with any disadvantages?

I think this comes back to my reasoning in the PR description. You can mimic the look and feel of the native UIBarButtonItems with Pressable/Touchable but I think it is difficult to get it exactly right. For example the UIBarButtonItemStyleProminent would be difficult to recreate with a custom view. Or setting the correct accessibility props. Using "the real" UIBarButtonItem also comes with the advantage that if Apple makes any changes to them no updates should be needed compared to using custom views.

Then there is also the disadvantage that custom views are not rendered in the "excessive items menu".

@johankasperi
Copy link
Author

johankasperi commented Oct 7, 2025

Once again thank you for your review @kkafar! I think I've adressed all of your comments 😊

Any news/decisions on the API for icon/sfsymbol?

@johankasperi johankasperi requested a review from kkafar October 7, 2025 21:28
Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, answered few open questions. I wanna say that for some reason I'm seeing "outdated" code version in the review screen, e.g. seeing the screenId, which should be gone. This might be a GitHub doing, unless something went wrong during force pushing / rebasing.

image

navitem.scrollEdgeAppearance = scrollEdgeAppearance;
#if !TARGET_OS_TV
navitem.hidesBackButton = config.hideBackButton;
navitem.leftItemsSupplementBackButton = config.backButtonInCustomView;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, thanks!

Comment on lines 52 to 56
if (_barButtonItem != nil) {
dispatch_async(dispatch_get_main_queue(), ^{
[self->_barButtonItem setHidesSharedBackground:self->_hidesSharedBackground];
});
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for verifying it. The mounting stage code (including prop updates) on both archs should be confined to UI thread, therefore we should be safe here.

Comment on lines 52 to 56
if (_barButtonItem != nil) {
dispatch_async(dispatch_get_main_queue(), ^{
[self->_barButtonItem setHidesSharedBackground:self->_hidesSharedBackground];
});
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can navigate to this commit & see the changes, but when reviewing I still see the old code. Is this GitHub glitch or was this commit lose somewhere in rebase?


To configure `ScreenStackHeaderRightView` or `ScreenStackHeaderLeftView` use `<BarButtonItemCustomView<` as a child. `<BarButtonItemCustomView>` component that comes from react-native-screens supports these properties:

- `hidesSharedBackground?: boolean` - Hide shared background (iOS 26+). Read more: https://developer.apple.com/documentation/uikit/uibarbuttonitem/hidessharedbackground
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point referring to the search bar here. I dislike distinguishing "custom subview" type from the rest of the API, however. Until now a way to render a custom view inside a header was to wrap it with ScreenStackHeaderSubview{Left,Rigth,Center} component. Most likely search bar was treated special, because it was a fully native view, w/o any react views inside, managed by the native side. Right now, we would create new way to render a custom view, IMO unnecessarily creating new ways of doing it. While I get that the hidesSharedBackground prop will not apply to all the possible contents I still think that its better way to go. Please note, that e.g. you could render SearchBar inside BarButtonInCustomView and still misuse the API, possibly breaking searchbar.
I want the hidesSharedBackgroundProp to live directly on header subview. In context where its application does not make sense - it should be a noop.

I feel like we could bikeshed here for a long time, therefore let's settle with putting in on ScreenStackHeaderSubview.

@kkafar
Copy link
Member

kkafar commented Oct 8, 2025

As per image / icon APIs, I've got a chat with expo-router team & we agreed on pushing the breaking changes to bottom tabs implementation. Therefore I think we should go directly for the approach from https://github.com/software-mansion/react-native-screens/pull/3214/files#diff-b4004b216c931d82615030eec09328c3405449f51cc1f96c83625301b983c805. I'm only thinking about flattening the type, because the API we introduce here applies solely to iOS, so we wouldn't have, so we would have icon prop, which takes PlatformIconIOS | PlatformIconShared. And specifying it would require the user to pass object with the type & appropriate payload.

@johankasperi
Copy link
Author

As per image / icon APIs, I've got a chat with expo-router team & we agreed on pushing the breaking changes to bottom tabs implementation. Therefore I think we should go directly for the approach from https://github.com/software-mansion/react-native-screens/pull/3214/files#diff-b4004b216c931d82615030eec09328c3405449f51cc1f96c83625301b983c805. I'm only thinking about flattening the type, because the API we introduce here applies solely to iOS, so we wouldn't have, so we would have icon prop, which takes PlatformIconIOS | PlatformIconShared. And specifying it would require the user to pass object with the type & appropriate payload.

Thank you! I will implement that asap

@johankasperi
Copy link
Author

As per image / icon APIs, I've got a chat with expo-router team & we agreed on pushing the breaking changes to bottom tabs implementation. Therefore I think we should go directly for the approach from https://github.com/software-mansion/react-native-screens/pull/3214/files#diff-b4004b216c931d82615030eec09328c3405449f51cc1f96c83625301b983c805. I'm only thinking about flattening the type, because the API we introduce here applies solely to iOS, so we wouldn't have, so we would have icon prop, which takes PlatformIconIOS | PlatformIconShared. And specifying it would require the user to pass object with the type & appropriate payload.

Implemented the icon prop in this commit a3a4e5f

@johankasperi johankasperi requested a review from kkafar October 8, 2025 21:03
Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Great job. Thank you! 🙌🏻

I'll do some more testing, but most likely I'll merge it today.

Comment on lines +198 to +202
- (UIBarButtonItem *)getUIBarButtonItem
{
if (_barButtonItem == nil) {
_barButtonItem = [[UIBarButtonItem alloc] initWithCustomView:self];
[self configureBarButtonItem];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this creates retain cycle. Subview retains back button & back button retains subview. I don't see a place where we break it -> most likely this leaks memory.

The subview does not have to retain the associated bar button item. Factory method in header config will be completely enough.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the subview does not retain the associated bar button item, then the bar button item will not be updated if the subview rerenders with a changed hidesSharedBackground value.

Comment on lines +111 to +127
export type PlatformIconShared = {
type: 'imageSource';
imageSource: ImageSourcePropType;
};

export type PlatformIconIOSSfSymbol = {
type: 'sfSymbol';
name: string;
};

export type PlatformIconIOS =
| PlatformIconIOSSfSymbol
| {
type: 'templateSource';
templateSource: ImageSourcePropType;
}
| PlatformIconShared;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for me: Need to move to shared directory, so that the PR with bottom tabs icon api refactor can use these.

@johankasperi
Copy link
Author

Looks good. Great job. Thank you! 🙌🏻

I'll do some more testing, but most likely I'll merge it today.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.